Extend pcntl_waitid with rusage parameter #15921
Conversation
vrza
force-pushed
the
waitid-rusage
branch
11 times, most recently
from
729ced4 to
ed5fcbd
Compare
IMHO, I would not worry that much about it. This PR qualifies more for the next major release. Ideally, the existing PR doc should be merged during the 8.4 doc rollercoaster. Merging this PR later and adding relevant infos in the doc. |
vrza
force-pushed
the
waitid-rusage
branch
2 times, most recently
from
9c27dde to
2254c05
Compare
That's fine, as this extension is fully backwards compatible with existing code.
Cool, all comments in that doc PR have been addressed and it's ready for another round of review. |
|
The FreeBSD test pass for me locally otherwise. |
|
They pass for me locally as well. Is there an easy way to see test logs from the Cirrus CI run? |
|
Changed unit test code to synchronize parent and child on every step. |
vrza
force-pushed
the
waitid-rusage
branch
5 times, most recently
from
f515815 to
6166f2c
Compare
|
Found a rare race condition in the unit test, where the child could receive a signal just after signal_dispatch, but just before sleep, in which case the signal would would not interrupt the sleep, and the test would take a long time to complete. Most direct solution I could think of was replacing this code with a quick (starting at 100 ns) exponential backoff loop, so that the race condtion is neutralized, but the unit test should still complete in milliseconds. @devnexen are you able to run that FreeBSD CI workflow again? |
|
Yes the test works for me locally. |
|
@devnexen Okay, great. I also had a colleague test it on macOS, macOS provides (undocumented) POSIX-compatible waitid, but doesn't support I self-reviewed one more time, and it looks good to go, but let's let others take a look as well. //cc @bukka @kocsismate |
|
I will take a proper look later. It's too late for PHP 8.4 so this should wait after branching it out anyway. |
|
Bumping this up for code review since it's been some time. @devnexen @bukka @petk @kocsismate @Girgias |
|
As for me, the code itself looks good. Maybe @kocsismate if possible double check the stub part ? |
If you could rebase/regenerate the stub, would be nice thx. |
|
Rebased, regenerated stub, tests passed. |
| /** @param array $info */ | ||
| function pcntl_waitid(int $idtype = P_ALL, ?int $id = null, &$info = [], int $flags = WEXITED): bool {} | ||
| /** | ||
| * @param array $info |
There was a problem hiding this comment.
not sure if we have to bother with these thus asking the stub wizard assistance :) once confirmed I ll merge shortly. Thanks for your work.
This functionality is not part of the POSIX interface. - On FreeBSD, the wait6 system call provides it - On Linux, the raw waitid system call provides it (glibc does not) Changed int to pid_t
Enhancement to #14616.
This functionality is not part of the POSIX waitid interface.
wait6system call provides itwaitidsystem call provides it, but it is not part of the interface exposed by glibc.Conceptually, this is similar to how
pcntl_waitandpcntl_waitpidextend the POSIX interface to fetch rusage info, where supported by the host OS.Requesting code review @devnexen @bukka @petk @kocsismate @Girgias
Documentation PR for pcntl_waitid is not merged yet, I can add to that open PR, or document this in a separate one, depending on what PR we merge first.
[edit] Created a documentation PR.